fix connection not release bug #353
Conversation
|
|
| @@ -171,6 +171,8 @@ def authenticate(self, user_name, password): | |||
| try: | |||
| resp = self._connection.authenticate(user_name, password) | |||
| if resp.error_code != ErrorCode.SUCCEEDED: | |||
| self._connection.is_used = False | |||
There was a problem hiding this comment.
the transport is not closed, prefer self.close()
There was a problem hiding this comment.
Is it necessary to close the connection here? I tend to release the connection back to the connection pool and wait for the next authenticate
nebula3/gclient/net/Connection.py
Outdated
| @@ -171,6 +171,8 @@ def authenticate(self, user_name, password): | |||
| try: | |||
| resp = self._connection.authenticate(user_name, password) | |||
| if resp.error_code != ErrorCode.SUCCEEDED: | |||
| self._connection.is_used = False | |||
| self._connection = None | |||
There was a problem hiding this comment.
Authenticate failed does not mean the connection is broken, Why set _connection = None, we can just set _connection.is_used as False.
Let's see the connection pool how to call authenticate function:
def get_session(self, user_name, password, retry_connect=True):
"""get session
:param user_name: the user name to authenticate graphd
:param password: the password to authenticate graphd
:param retry_connect:
:return: Session
"""
connection = self.get_connection()
if connection is None:
raise NotValidConnectionException()
try:
auth_result = connection.authenticate(user_name, password)
return Session(connection, auth_result, self, retry_connect)
except Exception:
raise
the auth operation is undependent with connection, so auth failed does not Influence the connection unless there are IO Error.
There was a problem hiding this comment.
I refer to the code under release function of session.py. I think you may right, self._connection.is_used = False is enought here
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #353 +/- ##
=======================================
Coverage 75.36% 75.36%
=======================================
Files 19 19
Lines 2724 2724
=======================================
Hits 2053 2053
Misses 671 671 ☔ View full report in Codecov by Sentry. |
|
@HarrisChu @Nicole00 kindly check the change now, what do you think? |
* fix connection not release bug * Address Nicole00's comment as agreed in review --------- Co-authored-by: Wey Gu <weyl.gu@gmail.com>
What type of PR is this?
What problem(s) does this PR solve?
Issue(s) number:
Description:
When authenticate to graphd failed,the status of the connection has not been modified, resulting in the inability to continue using it in the future.I encountered this issue and thinking this commit can fix it.
How do you solve it?
Modify connection status
is_usedto FalseSpecial notes for your reviewer, ex. impact of this fix, design document, etc: